-
Notifications
You must be signed in to change notification settings - Fork 192
Added feature to control http body behavior #821
Conversation
davidfowl
commented
Apr 20, 2017
- Added a flag to allow synchronous IO
- Added a flag to allow synchronous IO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting a global flag on KestrelOptions, not a per request flag. Or do you plan to do both?
I assume there is more to this PR than just this? |
Both.
This is just adding the feature. I don't plan to push all at once. Feature first, then Kestrel change once it goes in. |
@@ -0,0 +1,17 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright
@@ -0,0 +1,17 @@ | |||
using System; | |||
using System.Collections.Generic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you using from these namespaces?
/// <summary> | ||
/// Controls the IO behavior for the <see cref="IHttpRequestFeature.Body"/> and <see cref="IHttpResponseFeature.Body"/> | ||
/// </summary> | ||
public interface IHttpBodyControlFeature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the naming BodyControl. Maybe IHttpBodyIOOptionsFeature, IHttpBodyIOFeature, IHttpBodyIOCapabilitiesFeature?
@Tratcher Is HttpSysServer going to the same global flag? It would be pretty simple to disallow sync IO using some middleware to wrap the request and response body streams. The perf hit won't be too bad if the wrapping stream is pooled and lazily initialized. It would also ensure that a server switch doesn't suddenly cause a bunch of Read/Write calls to fail. |
Still don't like the naming, but functionally it's fine. |
@DamianEdwards do you have any naming suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@halter73 yes, HttpSys could add the same global and feature flags, but there's no bug filed for it. SyncIO is just as bad for HttpSys because you can block in native and can't cleanly cancel.
|